Skip to content

Conversation

@jmorrell-cloudflare
Copy link
Contributor

@jmorrell-cloudflare jmorrell-cloudflare commented Oct 9, 2025

Follow on to #5251

Add the following attributes to the existing spans:

durable_object_storage_kv_get

db.system.name
db.operation.name
cloudflare.durable_object.kv.query.keys
cloudflare.durable_object.kv.query.keys.count

durable_object_storage_kv_list

db.system.name
db.operation.name
cloudflare.durable_object.kv.query.start
cloudflare.durable_object.kv.query.startAfter
cloudflare.durable_object.kv.query.end
cloudflare.durable_object.kv.query.prefix
cloudflare.durable_object.kv.query.reverse
cloudflare.kv.query.limit

durable_object_storage_kv_put

db.system.name
db.operation.name
cloudflare.durable_object.kv.query.keys
cloudflare.durable_object.kv.query.keys.count

durable_object_storage_kv_delete

db.system.name
db.operation.name
cloudflare.durable_object.kv.query.keys
cloudflare.durable_object.kv.query.keys.count
cloudflare.durable_object.kv.response.deleted_count

@jmorrell-cloudflare jmorrell-cloudflare force-pushed the jmorrell/do-sql-kv-attributes branch from b64842e to c7817f7 Compare October 23, 2025 22:59
@jmorrell-cloudflare jmorrell-cloudflare marked this pull request as ready for review October 23, 2025 23:00
@jmorrell-cloudflare jmorrell-cloudflare requested review from a team as code owners October 23, 2025 23:00
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use instrumentation-tail-worker.js? That might not have been around when you first wrote this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. It feels a little weird to be pulling it in from ../../cloudflare/internal/test, but that's no reason not to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workerd/jsg/util.c++:393: error: e = kj/filesystem.c++:319: failed: expected parts.size() > 0 [0 > 0]; can't use ".." to break out of starting directory

Actually that breaks everything 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instrumentation-tail-worker.js?

That applies to src/cloudflare/internal/test/instrumentation-test-helper.js, I believe Mar meant src/workerd/api/tests/instrumentation-tail-worker.js. This is being used in several other tests such as src/workerd/api/kv-test.wd-test already, so we can hopefully use it here too.
(Having competing instrumentation-test-helper.js and instrumentation-tail-worker.js implementations is highly confusing, we plan to merge them soon).

@jmorrell-cloudflare jmorrell-cloudflare force-pushed the jmorrell/do-sql-kv-attributes branch from c7817f7 to 067e2fa Compare October 27, 2025 22:56
@codspeed-hq
Copy link

codspeed-hq bot commented Oct 27, 2025

CodSpeed Performance Report

Merging #5277 will not alter performance

Comparing jmorrell/do-sql-kv-attributes (fda1548) with main (48990e7)

Summary

✅ 33 untouched
⏩ 9 skipped1

Footnotes

  1. 9 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

compatibilityDate = "2025-08-01",

compatibilityFlags = ["enable_ctx_exports", "nodejs_compat"],
compatibilityFlags = ["enable_ctx_exports", "nodejs_compat", "streaming_tail_worker", "tail_worker_user_spans"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: tail_worker_user_spans has been obsoleted and is no longer needed anywhere

@fhanau
Copy link
Contributor

fhanau commented Oct 28, 2025

LGTM otherwise, just make sure to use one of the existing tail workers here as suggested, that will make the test easier to maintain in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants